Conversation
- LoginResult sealed interface 추가 (Success, Cancel, Failure) - LoginProviderFactory 추가하여 로그인 타입에 따른 Provider 반환 - LoginProvider interface 추가하여 login, logout 기능 정의
📝 WalkthroughWalkthrough이 PR은 Google 기반 로그인 기능을 새로 도입하고 관련 인프라를 연결합니다. 도메인에 로그인 인터페이스/타입을 추가하고, 데이터 계층에 AuthRepository 구현 및 토큰 저장을 도입했으며, feature/login 모듈에 Google 자격증명 흐름(GoogleLoginProvider), UI(로그인 버튼·화면), ViewModel, DI 바인딩, 문자열/리소스 및 Gradle 설정을 추가했습니다. 또한 네트워크 요청 모델과 엔드포인트 일부, 네비게이션 시작점을 로그인으로 변경하고, 기본 토큰 값을 빈 문자열로 초기화했습니다. Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~50 minutes 상세 리뷰긍정적 관찰
개선/검토 권장 사항 (왜 → 어떻게)
질문형 코멘트 (토론 권장)
마무리 메모 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In `@core/navigation/src/main/java/com/twix/navigation/AppNavHost.kt`:
- Around line 24-28: The startDestination in AppNavHost is hard-coded to
LoginGraph causing authenticated users to see the login screen; change
AppNavHost's start calculation to query the auth state (e.g., call
authRepository.hasValidToken() or equivalent) and choose NavRoutes.HomeGraph
when a valid token exists, otherwise NavRoutes.LoginGraph; additionally update
LoginRoute (and/or LoginViewModel) to observe LoginUiState.isLoggedIn on entry
and immediately navigate to NavRoutes.HomeGraph via navController if already
logged in so the LoginRoute short-circuits; ensure references to AppNavHost,
startDestination, LoginRoute, LoginViewModel, LoginUiState.isLoggedIn, and
authRepository.hasValidToken()/auth check are updated accordingly.
In `@data/src/main/java/com/twix/data/repository/DefaultAuthRepository.kt`:
- Around line 13-24: login() currently lets network/API exceptions bubble up;
wrap the service call (e.g., service.googleLogin(LoginRequest(idToken)) inside
DefaultAuthRepository.login) with error handling that converts errors to a
domain-friendly return (either return a Result type or map exceptions to a
domain-specific error sealed class) and ensure tokenProvider.saveToken(...) is
only called on success; handle the LoginType.KAKAO branch consistently (do not
return prematurely) and propagate failures as a controlled Result or mapped
error instead of throwing.
In `@feature/login/build.gradle.kts`:
- Around line 8-28: The current buildConfigField uses
localProperties.getProperty("google_client_id") which injects the literal "null"
when the key is missing; update the logic around localProperties /
buildConfigField to (1) read fallback sources such as
project.findProperty("googleClientId") and System.getenv("GOOGLE_CLIENT_ID") and
use the first non-empty value, and (2) if no value is found, fail the build
early (e.g., throw a GradleException) instead of writing "null" into
BUILD_CONFIG; locate and modify the code around the localProperties variable and
the buildConfigField("String", "GOOGLE_CLIENT_ID", ...) assignment to implement
this check-and-fallback behavior.
In `@feature/login/src/main/java/com/twix/login/component/LoginButton.kt`:
- Around line 66-86: The KAKAO branch in LoginType.uiModel() currently reuses
Google assets (ImageVector.vectorResource(DesR.drawable.ic_google) and
stringResource(R.string.google_login_button_title)); update the LoginType.KAKAO
case to reference the proper KAKAO assets (e.g., DesR.drawable.ic_kakao and
R.string.kakao_login_button_title) and appropriate colors in the
LoginTypeUiModel, or if assets aren't ready, remove/disable the KAKAO branch (or
return a placeholder/hidden state) to avoid exposing incorrect branding; locate
and change the LoginType.uiModel() function and the LoginType.KAKAO case and
ensure LoginTypeUiModel construction uses the correct logo/title/band colors.
In `@feature/login/src/main/java/com/twix/login/di/LoginModule.kt`:
- Around line 16-33: The module currently registers only the GOOGLE provider
causing LoginProviderFactory.get() to throw for LoginType.KAKAO; fix by either
registering a KakaoLoginProvider in LoginModule (add a
single<LoginProvider>(named(LoginType.KAKAO.name)) {
KakaoLoginProvider(androidContext()) } and include it in the map passed to
LoginProviderFactory) or, if Kakao is not ready, prevent the UI from exposing it
by guarding the KAKAO branch in LoginButton (remove/disable the KAKAO button or
conditionally render it based on a flag/feature check) so LoginButton cannot
call into LoginProviderFactory for an unregistered LoginType.
In `@feature/login/src/main/java/com/twix/login/LoginViewModel.kt`:
- Around line 21-30: The Success branch in login(result: LoginResult) calls
authRepository.login(result.idToken, result.type) without handling exceptions,
so network/server errors cancel the coroutine and skip emitSideEffect; wrap the
login call in a try/catch (or use runCatching) inside the LoginResult.Success
branch in viewModelScope.launch so any thrown exception will result in
emitSideEffect(LoginSideEffect.ShowLoginFailToast) (and optionally log the
exception) while still preserving the successful-path emit when no error occurs.
In `@feature/login/src/main/java/com/twix/login/navigation/LoginNavGraph.kt`:
- Around line 23-32: LoginRoute의 navigateToOnBoarding 콜백이 빈 구현이라 온보딩 이동이 무동작합니다;
LoginNavGraph에서 LoginRoute 호출부의 navigateToOnBoarding을
navController.navigate(NavRoutes.OnBoarding.route)로 구현해 실제 온보딩 화면으로 이동시키거나, 온보딩이
미지원이면 LoginRoute 호출부와 호출자를 찾아 navigateToOnBoarding 콜백을 제거(또는 호출 지점에서 분기 처리)해
불필요한 사이드이펙트가 발생하지 않도록 정리하세요; 참조 심볼: LoginRoute, navigateToOnBoarding,
navController, NavRoutes.OnBoarding (또는 NavRoutes.* 온보딩 라우트 이름).
🧹 Nitpick comments (7)
core/design-system/src/main/res/drawable/ic_google.xml (1)
1-21: 하드코딩 색상은 테마/다크모드 대응에 취약합니다.지금처럼 벡터 내부에 색상을 직접 박으면 테마·다크 모드에서 일관된 관리가 어렵고, 배경 대비가 낮아질 위험이 있습니다. 디자인 시스템에서 관리하는 색상 리소스로 분리하고, 필요 시 night 리소스를 추가하는 방식이 더 안전합니다. 구글 브랜드 컬러는 유지하되 색상 리소스로 추출하는 방식으로 정리해보실래요?
✅ 제안 변경(색상 리소스 참조)
- android:fillColor="#4285F4" + android:fillColor="@color/google_blue" ... - android:fillColor="#34A853" + android:fillColor="@color/google_green" ... - android:fillColor="#FBBC05" + android:fillColor="@color/google_yellow" ... - android:fillColor="#EA4335" + android:fillColor="@color/google_red"필요하다면
core/design-system/src/main/res/color/google_colors.xml에 정의하고,values-night로 대비 조정도 추가하는 방향을 추천드립니다. As per coding guidelines, 색상과 테마가 디자인 시스템에 포함되는가?domain/src/main/java/com/twix/domain/login/LoginProvider.kt (1)
3-7: 도메인 레이어에 적합한 인터페이스 설계입니다.Android 프레임워크 의존성 없이 순수 Kotlin 인터페이스로 정의되어 Domain 모듈 가이드라인을 잘 준수하고 있습니다.
한 가지 고려사항으로,
login()은LoginResult를 반환하고logout()은Result<Unit>을 반환하는데, 반환 타입의 일관성 측면에서logout()도LogoutResult같은 sealed interface로 정의하거나, 반대로login()도Result<LoginResult>로 통일하는 방안을 검토해 보실 수 있습니다. 현재 구조도 동작에는 문제가 없으니 선택적 개선 사항입니다.,
data/src/main/java/com/twix/data/repository/DefaultAuthRepository.kt (1)
20-20: KAKAO 로그인 타입의 early return 처리에 대한 명확한 의도 표시가 필요합니다.
LoginType.KAKAO -> return은 현재 미구현 상태를 나타내는 것으로 보입니다. 향후 유지보수를 위해 TODO 주석을 추가하거나,NotImplementedError를 던지는 것이 의도를 더 명확히 전달할 수 있습니다.현재 방식은 KAKAO 로그인 시 아무런 동작 없이 조용히 성공한 것처럼 보일 수 있어 디버깅을 어렵게 만들 수 있습니다.
💡 개선 제안
val response = when (type) { LoginType.GOOGLE -> service.googleLogin(LoginRequest(idToken)) - LoginType.KAKAO -> return + LoginType.KAKAO -> TODO("카카오 로그인 구현 예정") }feature/login/src/main/java/com/twix/login/google/GoogleLoginProvider.kt (2)
94-102: Silent login 결과 처리 로직 개선을 고려해보세요.현재
Failure일 때만 explicit 로그인을 시도하고,Cancel인 경우에는 그대로 반환합니다. Silent 요청에서Cancel이 발생하는 경우(예: 저장된 계정이 없을 때)에도 explicit 로그인으로 전환하는 것이 사용자 경험에 더 좋을 수 있습니다.💡 개선 제안
private suspend fun getGoogleCredentialResult(): GoogleCredentialResult { val silent = request(silentRequest) - return if (silent is GoogleCredentialResult.Failure) { + return if (silent !is GoogleCredentialResult.Success) { request(explicitRequest) } else { silent } }단, 사용자가 명시적으로 취소한 경우와 계정이 없어서 실패한 경우를 구분해야 한다면 현재 로직이 맞을 수 있습니다. 의도된 동작인지 확인 부탁드립니다.
33-40: CredentialManager와 serverClientId를 테스트 가능하게 주입받는 것을 고려해보세요.현재
CredentialManager와serverClientId가 클래스 내부에서 직접 생성/참조되어 단위 테스트 작성이 어렵습니다. 테스트 용이성을 위해 생성자 주입 방식을 고려해볼 수 있습니다.💡 개선 예시
class GoogleLoginProvider( private val context: Context, private val credentialManager: CredentialManager = CredentialManager.create(context), private val serverClientId: String = BuildConfig.GOOGLE_CLIENT_ID, ) : LoginProvider { // ... }현재 구조도 동작에는 문제없으나, 향후 테스트 코드 작성 시 개선하면 좋겠습니다.
feature/login/src/main/java/com/twix/login/LoginScreen.kt (2)
51-55: 로그인 로직을 ViewModel 내부로 이동하는 것을 고려해보세요.현재
LoginRoute에서coroutineScope.launch를 사용하여 로그인을 처리하고 있습니다. MVI 패턴에서는 비즈니스 로직을 ViewModel에서 처리하는 것이 권장됩니다.현재 방식의 문제점:
- 화면 회전 등 configuration change 시 코루틴이 취소될 수 있음
- 로그인 진행 상태(loading)를 UI에 반영하기 어려움
- View 레이어에 비즈니스 로직이 노출됨
💡 개선 방향
ViewModel에서
LoginProviderFactory를 주입받아 처리하면viewModelScope에서 안전하게 코루틴을 관리할 수 있습니다:// LoginRoute에서 LoginScreen { type -> viewModel.dispatch(LoginIntent.RequestLogin(type)) } // ViewModel에서 fun dispatch(intent: LoginIntent) { when (intent) { is LoginIntent.RequestLogin -> { viewModelScope.launch { val result = loginProviderFactory[intent.type].login() dispatch(LoginIntent.Login(result)) } } // ... } }현재 구조도 동작하지만, 로딩 상태 표시 등 추가 기능 구현 시 ViewModel로 이동하는 것이 좋겠습니다. As per coding guidelines, 단방향 데이터 플로우(Intent → ViewModel → State → View)가 유지되어야 합니다.
71-73: 주석 처리된 코드를 정리해주세요.
LoginType.entries.forEach코드가 주석 처리되어 있습니다. 현재 GOOGLE만 지원하는 것이 의도된 동작이라면, 주석 코드를 제거하거나 TODO 주석으로 향후 계획을 명시하는 것이 좋습니다.💡 개선 제안
Column( // ... ) { - // LoginType.entries.forEach { type -> + // TODO: 다른 소셜 로그인 추가 시 LoginType.entries.forEach 활용 LoginButton(type = LoginType.GOOGLE, onClickLogin = onClickLogin) - // } }또는 주석을 완전히 제거하고 깔끔하게 유지하세요.
dogmania
left a comment
dogmania
left a comment
There was a problem hiding this comment.
고생하셨습니다! 지금 ViewModel이랑 Repository 코드 보면 safeApiCall이랑 BaseViewModel 메서드 안 쓰고 통신을 처리하고 있는데 이거 나중에 수정해주세요! 지금 제가 홈 화면 테스트 하면서 코드 수정한 것도 있어서 60번 이슈 develop에 반영되고 나서 바꾸면 될 거 같아요
일단은 지금 테스트를 빨리 해봐야 해서 머지할게요!
2 participants
이슈 번호
#57
리뷰/머지 희망 기한 (선택)
작업내용
결과물
default.mp4
리뷰어에게 추가로 요구하는 사항 (선택)
앱에서 사용하는 외부 소셜 로그인이 변경 되었을 때 UI에서 발생하는 변경을 최소화하고 싶어서 소셜 로그인을 추상화 해봤어 !